-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ テーブル ] 縦積み機能を追加 #2410
[ テーブル ] 縦積み機能を追加 #2410
Conversation
…ature/table/align-vertical
@mtdkei 2人め確認をどなたかお願いします |
src/extensions/core/table/style.scss
Outdated
&[data-align-vertical-breakpoint="table-align-vertical-mobile"] { | ||
@media (max-width: 575.98px) { | ||
@include align-vertical-table; | ||
} | ||
} | ||
|
||
&[data-align-vertical-breakpoint="table-align-vertical-tablet"] { | ||
@media (max-width: 991.98px) { | ||
@include align-vertical-table; | ||
} | ||
} | ||
|
||
&[data-align-vertical-breakpoint="table-align-vertical-pc"] { | ||
@include align-vertical-table; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&[data-align-vertical-breakpoint="table-align-vertical-mobile"] {
これだと align-vertical がダブってて冗長だから
&[data-table-vertical-breakpoint="mobile"] {
でいいんじゃないかと思うけどどう?
あと、個人的には align は要素が左右だったり上下に”揃う(寄せる)”イメージなので、vertical-breakpoint だけで"縦積み"は伝わると思うけどどうかな?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtdkei と...思ったけど スクロールの value が table-scrollable-mobile だからあわせた方がいいのか...
じゃ、
&[data-vertical-breakpoint="table-vertical-mobile"] {
でどうでしょう?
え?「ちっさ! align 入ってるくらいええやろ!」って?
うん...まぁ...うん...そんな気もする...けど...ほら...先述の通り align だとテーブルの中の要素の上下揃えかなというイメージで "縦積み" とはニュアンスが違うかなと感じたのですがどうでしょう...(汗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
それか &[data-cell-align-vertical-breakpoint="table-cell-align-vertical-mobile"] { か...
あ、そうだ
&[data-cell-vertical-breakpoint="table-cell-vertical-mobile"] {
でいいんじゃない? align のところ全部 cell に書き換えないとあかんけど...
ど...どどど...どうですかね?
※反対意見歓迎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kurudrive
色々考えていただきありがとうございます。
&[data-cell-vertical-breakpoint="table-cell-vertical-mobile"] {
が良さそうですね!書き換えてみます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kurudrive
関数名、ラベル名を調整しました。
src/extensions/core/table/style.js
Outdated
table.removeAttribute('data-align-vertical-breakpoint'); | ||
} else { | ||
// alignVertical が ON の場合 | ||
table.classList.add('is-style-vk-table-align-vertical'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtdkei ありがとうございます。
VK Block Patterns の .vk-table--mobile-block とバッティングする(パターンやデモサイトデータなど、何気に使われてるサイトがかなり多い)ので、
on の場合は vk-table--mobile-block も存在してたら削除した方が無難かなと思います。
@mtdkei 変更ありがとうございます! ※ もちろん明日以降で可 |
@kurudrive |
@mtdkei 編集画面の対応確認いたしました。ありがとうございます! ↓の件も対応よろしくお願いいたします ( ̄人 ̄) |
@kurudrive |
@mtdkei ありがとうございました! |
チケットへのリンク / 変更の理由(元のissueがあればリンクを貼り付ければOK)
#2406
どういう変更をしたか?
コアのテーブルブロックで縦積みにする機能を追加しました。
水平方向スクロール機能のように「縦積みのブレイクポイント」が選べるようになっています。
また、これに合わせてVKアイコンの色がパネルごとに変更されるようにしました。
スクリーンショットまたは動画
変更前 Before
変更後 After
2025-01-22.8.20.49.mov
実装者の確認事項
実装者はレビュワーに回す前に以下の事を確認してチェックをつけてください。
プログラムの変更の場合
変更内容について何を確認したか、どういう方法で確認をしたかなど
feature/table/align-vertical
のブランチにし、npm run build
をする。(npm run build:cache や npm run build:dev だとscssがビルドされないことがあり、分割読み込みをONにすると追加したcssが効かなくなることがあるため。)
以下のHTML参照:
また、以下を確認しました。
レビュワーに回す前の確認事項
レビュワー確認方法・確認内容など
実装者と同じ確認を行なってください。
開発の方はコードの確認もお願いいたします。
レビュワー向け
レビュワーが確認して変更が反映されていない場合の確認事項
レビューしてみて意図した動作をしない場合は再度ビルドするなど以下の項目を確認してください。